Skip to content

Extend SerialPort #1601

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Extend SerialPort #1601

wants to merge 3 commits into from

Conversation

@sebastianbergt
Copy link

Description
Adding software flow control parameter xonoff to SerialPort, since otherwise I could not get my current board to run with labgrid.

Checklist

  • Documentation for the feature
  • Tests for the feature
  • The arguments and description in doc/configuration.rst have been updated

A library feature which other developers can use:

  • PR has been tested (with an Orin Linux Board)
  • Man pages have been regenerated

@sebastianbergt sebastianbergt force-pushed the feat/software-flow-control branch 2 times, most recently from 8ecd172 to 9e2ec49 Compare February 10, 2025 09:25
@sebastianbergt @Emantor
Extend SerialPort
8d1e7fa 
Adding software flow control parameter xonoff to SerialPort.

Signed-off-by: Sebastian Bergt <[email protected]>
@Emantor Emantor force-pushed the feat/software-flow-control branch from 15edde9 to 8d1e7fa Compare February 17, 2025 06:54
@Emantor Emantor self-assigned this Feb 17, 2025
@Emantor Emantor self-requested a review February 17, 2025 06:55
@codecov
Copy link

codecov bot commented Feb 17, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 55.8%. Comparing base (02fa1ef) to head (ee3c5d1).
⚠️ Report is 150 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files 
@@           Coverage Diff            @@
##           master   #1601     +/-   ##
========================================
- Coverage    55.8%   55.8%   -0.1%     
========================================
  Files         170     170             
  Lines       13377   13382      +5     
========================================
+ Hits         7469    7471      +2     
- Misses       5908    5911      +3
Flag Coverage Δ
3.10 56.1% <100.0%> (-0.1%) ⬇️
3.11 56.1% <100.0%> (-0.1%) ⬇️
3.12 56.1% <100.0%> (-0.1%) ⬇️
3.13 56.1% <100.0%> (-0.1%) ⬇️
3.9 56.1% <100.0%> (-0.1%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Emantor
Emantor previously approved these changes Feb 17, 2025
View reviewed changes
Copy link
Member

@Emantor Emantor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good, usage.rst needs to be updated.

@Emantor Emantor removed their assignment Feb 17, 2025
@sebastianbergt
Copy link
Author

sebastianbergt commented Feb 19, 2025 •
edited
Loading

@Emantor thank you for the quick review and comment :)

Changes look good, usage.rst needs to be updated.

I am happy to update usage.rst where you see fit, but it seems to me it shall have the most simple examples to keep first time users on the right path.

@Emantor
Copy link
Member

Emantor commented Feb 19, 2025 •
edited
Loading

@Emantor thank you for the quick review and comment :)

Changes look good, usage.rst needs to be updated.

I am happy to update usage.rst where you see fit, but it seems to me it shall have the most simple examples to keep first time users on the right path.

I meant the CI failure which complains about an argument mismatch.

@sebastianbergt
Updated usage.rst
a68e340 
Signed-off-by: Sebastian Bergt <[email protected]>
@sebastianbergt
adding xonoff in overview.rst
ee3c5d1 
Signed-off-by: Sebastian Bergt <[email protected]>
@sebastianbergt sebastianbergt force-pushed the feat/software-flow-control branch from a9d6c9e to ee3c5d1 Compare February 19, 2025 14:33
@sebastianbergt
Copy link
Author

@Emantor when testing xonoff with the exporter, I am running into issues:

Could you give me a hint how to fix this, please?

add resource example_group/example_device: USBSerialPort/OrderedDict([('match', OrderedDict([('ID_VENDOR', 'FTDI'), ('ID_USB_MODEL', 'FT232R_USB_UART')])), ('speed', 115200), ('xonoff', True)])
Traceback (most recent call last):
  File "/home/labgrid/labgrid-venv/bin/labgrid-exporter", line 8, in <module>
    sys.exit(main())
  File "/home/labgrid/labgrid-venv/lib/python3.10/site-packages/labgrid/remote/exporter.py", line 1109, in main
    asyncio.run(amain(config), debug=bool(args.debug))
  File "/usr/lib/python3.10/asyncio/runners.py", line 44, in run
    return loop.run_until_complete(main)
  File "/usr/lib/python3.10/asyncio/base_events.py", line 649, in run_until_complete
    return future.result()
  File "/home/labgrid/labgrid-venv/lib/python3.10/site-packages/labgrid/remote/exporter.py", line 1032, in amain
    await exporter.run()
  File "/home/labgrid/labgrid-venv/lib/python3.10/site-packages/labgrid/remote/exporter.py", line 844, in run
    await self.add_resource(group_name, resource_name, cls, params)
  File "/home/labgrid/labgrid-venv/lib/python3.10/site-packages/labgrid/remote/exporter.py", line 1003, in add_resource
    res = group[resource_name] = export_cls(
  File "<attrs generated init labgrid.remote.exporter.SerialPortExport>", line 8, in __init__
  File "/home/labgrid/labgrid-venv/lib/python3.10/site-packages/labgrid/remote/exporter.py", line 200, in __attrs_post_init__
    self.local = USBSerialPort(target=None, name=None, **self.local_params)
TypeError: USBSerialPort.__init__() got an unexpected keyword argument 'xonoff'
Exception ignored in: <function SerialPortExport.__del__ at 0x7ec7decc1c60>

@Emantor
Copy link
Member

Emantor commented Feb 20, 2025

Is the labgrid inside of your venv running with this PR? It looks like the USBSerialPort does not know the keyword argument.

@sebastianbergt
Copy link
Author

sebastianbergt commented Feb 20, 2025 •
edited
Loading

Is the labgrid inside of your venv running with this PR? It looks like the USBSerialPort does not know the keyword argument.

Yes, I installed it from my branch like so:
pip install git+https://github.com/sebastianbergt/labgrid@feat/software-flow-control

EDIT: Found a mistake I made installing. Will try that way:
image

@Emantor
Copy link
Member

Emantor commented Feb 20, 2025

Note that you can also install into a local venv via pip install -e . inside the labgrid repository, should be under development setup in the documentation.

@sebastianbergt
Copy link
Author

Just changed the installation to venv, git clone ... and pip install.

Apparently it has the correct file, but the exporter keeps failing nevertheless, if I use the xonoff attribute :(

(labgrid-venv) labgrid@testpc:~$ cat labgrid-venv/lib/python3.10/site-packages/labgrid/resource/base.py
import attr

from ..factory import target_factory
from .common import Resource


@attr.s(eq=False)
class SerialPort(Resource):
    """The basic SerialPort describes port and speed

    Args:
        port (str): port to connect to
        speed (int): speed of the port, defaults to 115200
        xonxoff (bool): software flow control, defaults to False (=off)"""
    port = attr.ib(default=None)
    speed = attr.ib(default=115200, validator=attr.validators.instance_of(int))
    xonxoff = attr.ib(default=False, validator=attr.validators.instance_of(bool))

@sebastianbergt
Copy link
Author

sebastianbergt commented Feb 20, 2025 •
edited
Loading

Turned out to be a hard to spot typo xonoff vs xonxoff. So problem solved.

@Emantor Emantor requested a review from jluebbe June 19, 2025 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Emantor Emantor Emantor approved these changes

@jluebbe jluebbe Awaiting requested review from jluebbe

Assignees

@sebastianbergt sebastianbergt

@jluebbe jluebbe

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sebastianbergt @Emantor @jluebbe